-
Notifications
You must be signed in to change notification settings - Fork 67
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @dreamryx. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@dreamryx why do you think this PR is needed? Is there a specific bug to be fixed? The schema registration package is for predefined types and it has unit tests to verify the schemas actually load without errors such as stack overflow. |
@zhenghuiwang Thanks for your review. :) It's really a QA test case here:
Yes. As you said, currently the pre-defined schema are well designed and tested. But we need to add a guard to eliminate the concern in server:
This PR could help improve the server. Hope you can approve it. Thanks. |
@dreamryx I agree there can be theoretical circular reference problem for the schemas. Is there any specific example of schema that you want to include that has circular reference? IMHO we should not have circular reference at the first place and we should let the server panic (due to overflow) during start if there are. Circular reference is not just hard to write code to parse but also hard for human to consume. We should strive to make the schemas self-contain. |
@zhenghuiwang The circular reference is QA test case for now. I do not have an example to use circular reference. When there are complicated pre-defined schemas, it is hard for human to check circular reference and find which schema is wrong if circular reference comes up. The server crash with "stack overflow" message can not help user to know what happens and how to fix it quickly. I think it's the server's responsibility to help user check it and report the info to user. |
when server starts, proactive schema check and a clear error message can help indeed instead of panic message. |
There is already a unit test to test loading the predefined schemas. Schemas are predefined so that you can check errors before run time and the parsing code doesn't have to handle arbitrary schemas, e.g. ones with circular references. I think it is a fine assumption because it simplifies the parsing code and schema. These newly added code will run during sever start but won't find anything.(otherwise something is wrong with the releasing process) It will just print a nicer error message when the unit test fails. To me that is not the right reason to complicate the production code. |
What this PR does / why we need it:
Since the schema contains parent ref, we need to prevent closed loop. Otherwise error like "stack overflow" may comes.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #119
This change is